+reviewer: @lukecwik

It seems good in general that TestPipeline tests that the serialization of pipeline options works. In particular, this may be needed if we want to pass options through the Runner API to the actual runner.

Ok, then we need a way for tests to bypass this so specified options aren't dropped.

In the portable future, options are allowed/expected to be serialized on their way to a runner. So that means @JsonIgnore options are just a way to use PipelineOptions for Beam-irrelevant command line parsing. We shouldn't really encourage that, since mature/stanardized command line libraries are more appropriate.

@kennknowles Ah, ok. Thank you for the explanation!

Serialization of pipeline options should only matter between SDK and runner like Kenn says and not between test infrastructure and SDK.

I'm all for having a good way to do args parsing and dependency passing outside of PipelineOptions, I just don't see that being feasible while being backwards compatible. Also using two argument parsing systems is typically awkward so I can't see a migration away from using PipelineOptions, only a full swap to something else.

What I mean is that just using PipelineOptions to configure a user's main() program is not great.

The PipelineOptions absolutely should go through a serialization round trip before they reach the runner, since this is to spec. We should fix anything that breaks because of it.

@pgerv12 can you describe the particular problem that happened? I do think that existing runners may have a lot of dependencies on non-serializable options just for configuration. But for a non-Java SDKs the options have to come serialized, so we need to fix the runners to have all of their configuration specified in ways that can be sent that way.

@kennknowles The Streams Runner has a --jarsToStage option which it uses to add dependent jars to its application bundle (marked with JsonIgnore). When testing Beam 2.2, our IT is failing because Classes could not be found because the jars weren't present. Looking at the reason, our runner never received the String passed in through the beamTestPipelineOptions system property variable. The TestPipeline properly parses the variable and sets its member options correctly with #testingPipelineOptions. During the run phase, this serialization occurs making jarsToStage null, this then gets to our runner and doesn't add any jars cause none are specified.

I can see where the non-Java SDK need arises and it is problematic since this is purely Java SDK-based. A proper solution sounds like there needs to be two types of serialization: one for complete PipelineOptions transfer used for testing/allowing runners to get all configuration parameters (used for test infrastructure -> SDK -> Runner translation), and the second is the FasterXML Jackson serialization (Runner translation -> Runner runtime).

For reference, it sounds like JsonIgnore should differentiate between whether a DoFn needs it during execution or not (I understand this may be old documentation): https://beam.apache.org/documentation/sdks/javadoc/2.2.0/org/apache/beam/sdk/options/PipelineOptions.html

I see. This makes a lot of sense, and should be supported until we have a complete portability story.

For reference, in the portable model the Java SDK harness will execute the user's UDFs, so it would own options like this. Artifact staging goes over the "Artifact API" (just a proxy for staging via a runner-specific mechanism) and the Java construction side and Java execution side use it however they want, and just have to agree. So then @JsonIgnore is fine for files to stage again.

For your case, now, can you just make the option not @JsonIgnore? If that is a problem, them I'm OK with this change but we should develop a proper plan for TestPipeline to function appropriately for both styles of runner.

Yes, our workaround it to remove the annotations and things work well with the change. I'm glad we're having this discussion because I didn't know what the proper behavior should be.

jarsToStage sounds a lot like filesToStage. Note that filesToStage is not marked with @JsonIgnore for Flink/Spark/Dataflow.

Dropping the @JsonIgnore seems like the right approach but also consider migrating to use filesToStage and the same jar detection code which Flink/Spark/Dataflow all share as can be seen here:

Is it the right approach? My argument is that filesToStage should be marked with @JsonIgnore since it's not really needed at the worker side. If that is or isn't the case, looking at the FlinkPipelineOptions, getStateBackend() is marked @JsonIgnore and it sounds like if I wanted to run tests with a different backend then that wouldn't be doable from my test parameters because of this problem (or if I used a non-Java SDK, I could never be able to set the backend through parameters since my arguments would always be serialized away).

Edit: I understand for portability that serialization need to occur so I would say if the goal is portability then the @JsonIgnore should disappear entirely.

There are reasons why a user my actually want to know what files were staged and this could be one way for them to get it. The original reason why it was marked with @JsonIgnore was because of a test infrastructure limitation in Dataflow 1.x. I would prefer to have as few options as possible marked with @JsonIgnore so users don't need to treat PipelineOptions differently during construction vs execution.